Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autocomplete: Put best matches near input field. #1131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

apoorvapendse
Copy link

@apoorvapendse apoorvapendse commented Dec 11, 2024

This commit reverses the list that was originally
presented to the user while showing the typeahead
menu.

This makes sense since on mobile its easier to click on options closer to the input box, i.e. where your fingers are currently present, instead of pressing arrow keys to navigate through the options on a keyboard which is convenient on a desktop setup.

Hence we place the best matching options not at the top of the typeahead menu, but instead at the bottom for better reachability and convenience of the user.

CZO

Fixes #1121.

Here is a demo:
Screencast from 12-11-2024 12:38:19 PM.webm

image
image

@apoorvapendse apoorvapendse force-pushed the put-best-matches-near-fingers branch 3 times, most recently from a146a49 to bd3a205 Compare December 16, 2024 02:00
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @apoorvapendse! Left some comment.

This doesn't fix #1123 because the emoji picker (added in #1103) is a feature separate from autocompletion, so please update the PR comment and the commit message to reflect that.

test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
test/widgets/autocomplete_test.dart Show resolved Hide resolved
test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
// biohazardOptionLabel, this won't be rendered in the list initally since it is the 7th option.
];

await tester.enterText(composeInputFinder, 'hi :');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a comment here if it's also affected by #226.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not understand?

test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
@apoorvapendse apoorvapendse force-pushed the put-best-matches-near-fingers branch from bd3a205 to 129434b Compare December 18, 2024 03:04
@apoorvapendse
Copy link
Author

Hey @PIG208, thanks for reviewing the PR.
I've updated it and hope it meets your expectations.
Could you please elaborate on what kind of comment is expected as per this comment
Thanks.

@apoorvapendse apoorvapendse requested a review from PIG208 December 18, 2024 03:16
This commit reverses the list that was originally
presented to the user while showing the typeahead menu.

This makes sense since on mobile its easier to click
on options closer to the input box, i.e.
where your fingers are currently present,
instead of pressing arrow keys on a keyboard which is
true on a desktop setup.

Hence we place the best matching options
not at the top of the typeahead menu, but
instead put them at the bottom for better
reachability and convenience of the user.

Tests have been added to verify the
emoji and mention render behavior.

Fixes zulip#1121.
@apoorvapendse apoorvapendse force-pushed the put-best-matches-near-fingers branch from 129434b to b35ddbf Compare December 18, 2024 03:18

final expectedUserSequence = users;
// Options are filtered correctly for query
// TODO(#226): Remove this extra edit when this bug is fixed.
Copy link
Member

@PIG208 PIG208 Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #1131 (comment), I was referring to this comment here.

@PIG208 PIG208 self-assigned this Dec 18, 2024
@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Put best autocomplete matches at bottom of list, not top
2 participants